Repository navigation
Conversation
| processing, | ||
| response_send, | ||
| ); | ||
| drop(guard); |
There was a problem hiding this comment.
[P2] Keep corruption cleanup bounded
Dropping guard here releases admission while this request still performs disk reads, hashing, and quarantine writes in recheck_corrupt().
If quarantine stalls on a shard lock or slow filesystem, further challenges against the retained commitment can read the same corrupt file and start additional cleanup tasks. Different peer identities bypass the per-peer cooldown. The work budget charges only proof generation, and TaskTracker does not impose a concurrency limit, so cleanup can accumulate beyond the configured responder limit and occupy the shared blocking pool, delaying unrelated storage operations.
Please retain the guard until cleanup finishes (the response has already been sent), or hand cleanup to a bounded queue that deduplicates keys. A regression test could stall quarantine and submit further challenges to verify that pending cleanup stays bounded.
This finding follows from the admission and storage code paths; I did not reproduce blocking-pool exhaustion end to end. Validation on this commit: 1,232 library tests, 16 live audit-handler tests, and 19 adversarial commitment-audit tests passed.
dirvine
left a comment
dirvine
left a comment
There was a problem hiding this comment.
Review of 3f4337c3c037ae1844a1c13993504559650fc7f1
P2 — Keep corruption cleanup inside a bounded admission path
At src/replication/mod.rs:5279-5285, the round-one responder sends the reply, explicitly drops its admission guard, then awaits the corrupt-key rechecks. Those rechecks perform verifying disk reads and can await a shard write lane and quarantine I/O. They are no longer counted by either the global or per-peer responder limit.
If a write lane stalls while raw reads can still complete, subsequent admitted proofs can start more cleanup work despite earlier cleanup being outstanding. This can exceed the intended concurrency limit and place additional work in the shared blocking pool. Per-proof subtree geometry and the existing byte-budget/cooldown limit individual work and admission rate; they do not bound the outstanding cleanup count. FileStore::get_raw still reads the file directly, so a pending quarantine does not inherently prevent another retained-commitment proof from encountering it.
This corroborates the existing inline concern; I have not added a duplicate inline comment. This is a source-traced failure mode, not a measured production DoS or an executed resource-exhaustion reproduction.
Please keep the guard through cleanup after sending the response, or move cleanup into a separately bounded worker/queue with a defined full-queue policy (and preferably per-key deduplication). Add a regression which stalls cleanup and checks that further requests cannot accumulate cleanup beyond the chosen cap. A timeout alone is insufficient if it releases admission while already-started blocking work continues.
What passed
Executed locally on the reviewed head:
cargo test --locked --lib --features test-utils: 1,232 passed, 0 failed.cargo test --locked --features test-utils --test poc_audit_handler_live --test poc_commitment_audit_attacks: 16 live-handler tests and 19 adversarial commitment-audit tests passed.- Focused corruption/intact, storage, pointer-audit and budget tests also passed; these overlap the library suite, not additional unique test counts.
- Diff whitespace and format checks passed. GitHub's non-skipped checks were successful; optional Claude review was skipped.
The corruption handling itself looks sound: proof bytes remain honest, quarantine rechecks under the write lane before removal, and an auditor with no usable local references returns Idle rather than giving an undeserved pass or judging a peer using rotten bytes.
Testnet scope
Use verify_on_read=true (the default); disabling it also disables this repair trigger's verification. Quarantine/exclusion is not itself proof of successful repair: demonstrate valid refetch from an available replica, a later successful audit, intact/concurrent-repair controls, and the corrupt-auditor-reference case. Include stalled-cleanup admission in the test matrix. No testnet was launched as part of this review.
Panel reconciliation
Completed panel: storage seat APPROVE (no introduced storage-safety defect found); protocol/concurrency seat non-blocking for a controlled testnet, with hardening before broad rollout; independent GLM-5.2 APPROVE-WITH-CONCERNS. All identify the missing aggregate cleanup concurrency cap. The first two timed-out workers are not counted as completed seats. The replacement storage seat consulted an existing same-head review reference as well as source, so it is corroboration rather than fully blinded review.
Final adjudication: REQUEST_CHANGES on the P2 bound above, with explicit severity dissent from the protocol and GLM seats. Their rate-bound and per-task sequential-work observations are valid, but do not establish a bound on outstanding tasks when cleanup stalls. Being the tail of an already-tracked task rather than a separately spawned task does not change that: releasing admission allows new tracked tasks to overlap the old tails. Nor do passing tests reproduce this stalled-cleanup case. I therefore do not adopt the protocol seat's stronger claims that resource exhaustion is excluded or the risk is bounded in practice.
This is not a finding of data loss or incorrect peer punishment, and it does not prohibit a maintainer-approved isolated diagnostic testnet. It is a request to close the admission gap before calling the PR ready for acceptance/rollout. A dedicated bounded cleanup queue would preserve proof-response capacity better than holding the proof permit indefinitely; ensure queue admission itself does not create an unbounded population of waiting tasks.
Every audit reads chunk bytes through ChunkStore::get_raw, which does not check them against their address. A node whose chunk file has gone bad on disk therefore never notices from audits: it keeps committing the key and fails every subtree audit whose selected block contains it, and only a fetch of that exact key (the verifying get path) would take the file out of service and let replication repair it. In the other direction, an auditor whose own reference copy has gone bad fails honest peers in the responsible-chunk, possession and prune lanes. Responder: round 1 already hashes every leaf it reads, so a chunk leaf whose plain hash differs from its key is now reported in Round1Work::corrupt_keys. Once the reply has been sent and the admission permit released, the replication engine passes each one to the new ChunkStore::recheck_corrupt, which runs the existing verifying read: it re-reads the file under the shard write lane, removes it only if it is still wrong, drops the key from the view the next commitment is built from, and re-queues a legacy copy if there is one. The proof still carries the bytes that were read, so the auditor's verdict is unchanged. Auditor: the reference copy in the responsible-chunk, possession and prune lanes is now read with ChunkStore::get instead of get_raw, so a rotted local copy is taken out of service and the key skipped instead of failing the peer. A responsible audit in which no key could be checked against a good local copy is now idle rather than a pass, and keys_checked counts only the keys actually verified. Both sides follow the node's verify_on_read setting, like every other verifying read.
Round 1 of the subtree audit reports the committed chunks whose bytes no longer hash to their key, and the responder took each one out of service with a verifying read after sending its reply and releasing its admission permit. That cleanup ran outside every responder limit. The verifying read ends in a quarantine that waits for the chunk's shard write lane on a blocking-pool thread, and while that lane is stalled further proofs keep reading the same rotted file: an auditor can pin a retained commitment that still contains it, and a new peer identity escapes the per-peer cooldown. Each of those proofs parked another blocking thread, with nothing capping how many. The chunk store now keeps the reported keys in a FIFO queue, deduplicated against both the queue and the key being rechecked, and capped at 1024 keys, the most a single round-1 proof can cover. Reporting never waits. A key that does not fit is dropped, and the next audit that reads it reports it again, since it stays on disk and committed until it is removed. The replication engine runs one worker that rechecks the queue a key at a time, so however many audits report rot and however long a lane stalls, the rechecks hold at most one blocking thread between them. The worker stops at the shutdown token between rechecks, and is tracked with the engine's detached storage work, so shutdown waits for a recheck in progress rather than aborting it. Keys still queued at shutdown stay on disk and committed, so after a restart the next audit that reads them reports them again. Holding the round-1 permit through the cleanup would also have bounded it. But round 1 never takes a write lane, so two proofs over rotted chunks in a stalled shard would then have stopped the node answering every subtree audit until the lane freed. The round-1 task keeps its permit until it finishes, as it did before the cleanup was added, and only queues the keys. A regression test holds a shard's real write lane while 32 reports arrive and requires at most one blocking task in flight; with each report running its own recheck, as before, it sees 33. Another checks that everything the protocol's largest subtree can report fits an empty queue. A new end-to-end test rots a node's chunk files, audits it over the live wire, and requires the audit to fail and at least one rotted chunk it read to leave service. Three assertions in this branch's tests gain failure messages for the assert_is_empty lint that Rust 1.99 added and main now passes.
3f4337c to
ebfdd04
Compare
dirvine
left a comment
dirvine
left a comment
There was a problem hiding this comment.
APPROVE — re-review of ebfdd04
Hermes agent review, posted through the authorised GitHub account; not a separate personal review by David.
This supersedes my REQUEST_CHANGES on 3f4337c. The cleanup-admission blocker is resolved. No remaining actionable blockers identified at this exact head.
The completed same-head re-review panel (storage, concurrency/protocol, and independent GLM-5.2) agreed the blocker was resolved. GLM retained non-blocking concerns, not a change request. For this formal submission I rechecked the live head, current CI, and exact-head queue/worker source. No new panel or local test run is claimed today.
Fix confirmed
- A single production worker runs rechecks sequentially on the detached storage task tracker (
src/replication/mod.rs:2259-2261,3302-3315). CorruptReportscaps admitted keys at 1,024; its set includes both waiting and in-flight keys (src/storage/chunk_store.rs:237-265).report_corruptdoes not await disk work or queue capacity. It uses a short synchronous mutex section; overflow is explicitly dropped for later rediscovery. The worker removes the dedup entry only after the active recheck completes (src/storage/chunk_store.rs:815-860). Thus stalled cleanup cannot accumulate one cleanup task per subsequent audit.
Evidence and limits
The prior completed re-review at this unchanged SHA recorded 1,235 library tests and 42 selected integration/E2E tests passing, including stalled-quarantine, shutdown-drain and live-wire corruption coverage. These are prior-session results, not newly executed tests. All currently reported GitHub checks are passing.
Non-blocking caveats remain: repair verification follows verify_on_read; a permanently stalled storage operation can delay shutdown under the existing drain contract; dropped reports need later rediscovery; worker panic supervision is not added here. None reintroduces the original unbounded cleanup-admission defect.
Acceptance testing should still establish the full removal → valid refetch → successful subsequent audit sequence, with intact chunks preserved. The earlier local E2E established removal, not full repair convergence. This is code-review approval, not a claim that acceptance testing has passed or permission to merge/deploy. No merge or testnet launch performed.
Dev testnet evidence (T2) — run V2-1458, DEV-04, registry id 652, 2026-10-07Built from this PR's head Injection. At T0+1h, 50% of the chunk files on one service on each of 9 node VMs were overwritten in place with the bytes
Per-key read of the logs. Every key-bearing line named only rotted keys (2,330 removals, 1,923 detections, 27 Non-regression versus released 0.21.0 on the same shape (registry id 635): uploads 2,857/2,857; load 0.080 MiB/s per node (635: 0.075); node RSS p95 311→323 MiB (635: 317→337), none over 1 GiB; 0 failed units, restarts or upgrades. Observations, not against this PR. Six Full results with every command's verbatim output: Linear V2-1458; summary on V2-1382. 🤖 Generated with Claude Code |
4 participants
Linear issue
Closes V2-1382
Risk tier
Compatibility
Semver impact
Test evidence
Why: every audit reads chunk bytes with
get_raw, which does not check them. A node whose chunk file has gone bad on disk keeps committing it and fails every subtree audit that lands on it, and only a fetch of that exact key repairs it. In production, all 186 subtree round-1DigestMismatchfailures our fleet logged between 2026-09-25 and 2026-10-01 were against 10 community nodes, none against our own 774 (212,871 passes). The node behind most of them answered our fetches with its ownChunk verification failed, which is this state.What changes:
get, so a rotted local copy is taken out of service and that key skipped instead of failing an honest peer. A responsible audit in which no key could be checked is idle rather than a pass.New tests:
a_stalled_quarantine_holds_one_blocking_thread_however_many_reports_arrive: holds a shard's real write lane while the worker's quarantine waits on it, has 32 more reports of the rotted chunk arrive, and requires at most one blocking task in flight, every report returned, and the queue worked through once the lane frees. With each report running its own recheck, as on the previous head of this PR, it fails with33 rechecks held blocking threads behind one stalled write lane.corrupt_reports_are_deduplicated_and_cappedandthe_corrupt_queue_holds_everything_one_proof_can_report: dedup against the queue and the key in flight, the full-queue drop, the restart rule, and that everything a maximal proof (max_subtree_leaves(MAX_COMMITMENT_KEY_COUNT)keys) can report fits an empty queue.a_node_whose_chunks_rotted_takes_them_out_of_service_after_an_audit(e2e): rots a node's chunk files, audits it over the live wire, and requires the audit to fail, at least one rotted chunk it read to leave service, and every chunk removed to be unclaimed. With the engine's worker not started it fails withno rotted chunk the audit read was taken out of service.round_one_takes_a_rotted_committed_chunk_out_of_service,a_rotted_local_copy_neither_fails_nor_passes_the_peer,a_rotted_local_copy_is_not_used_to_judge_holders,recheck_corrupt_takes_a_rotted_file_out_of_service_and_spares_a_good_one,round_one_over_intact_chunks_keeps_every_chunk: unchanged from the previous head (the first four fail with the change reverted); the first now goes through the queue and worker.Suites, on this head:
cargo test --lib --features test-utils: 1235 passed.poc_audit_handler_live16/16,poc_commitment_audit_attacks19/19,poc_shutdown_lmdb_drain1/1,poc_bootstrap_stall3/3,migration_reclaims_disk2/2,migration_shared_volume5/5,migration_crash_safety5 passed (3 ignored, as on main).cargo test --test e2e --features test-utils -- --test-threads=1: 111 passed, 3 ignored (as on main), including the 6 subtree-audit tests.cargo clippy --all-targets --all-features -- -D warningson Rust 1.98 and 1.99,cargo fmt --check,cargo doc --all-featureswith warnings denied,RUSTFLAGS="-D warnings" cargo check --lib --tests --no-default-featureson 1.99, and the 1.95 MSRVcargo check --all-targets --all-features --locked: clean.Dev testnet: not run here. For the testnet: keep
verify_on_read=true(the default; turning it off also turns off this repair). A rotted chunk shows asSubtree audit: committed key … does not hash to its addresson the audited node, thenRemoved corrupt chunk file …; replication will repair it. A… chunk(s) found rotted were not queued for a recheckwarning means a burst outran the queue. Removal is not repair by itself: the evidence to look for is the key being fetched back from a replica and the next audit over it passing, plus intact controls staying in place.New dependency
none
ADR
https://github.com/WithAutonomi/ant-node/blob/main/docs/adr/ADR-0014-file-based-chunk-store-and-lmdb-retirement.md: a chunk whose bytes are proven wrong stops being claimed, so ordinary replication can repair it. This PR applies that existing rule to the reads audits make. The subtree audit's wire and proof formats are unchanged (https://github.com/WithAutonomi/ant-node/blob/main/docs/adr/ADR-0002-gossip-triggered-contiguous-subtree-audit.md); the auditor lanes now skip a corrupt local reference instead of judging a peer against it.
Mitigation / rollback
Revert the commits. Nothing persisted or on the wire changes. The effects are that chunks already proven corrupt are removed so replication can refetch them, and that auditors skip a corrupt local reference instead of judging a peer against it.